-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proof Of Concept] Enable plugin mode #335
Conversation
Hey Adrian — thanks for the proposal! It definitely did start a conversation 😊 The main thing we can say is that we’re moving toward a general app architecture that should end up achieving what you want here: we’re moving toward a “global shell” way of loading apps in an iframe, which will entail building each app in plugable way (i.e. without the header bar and with a communication channel from its parent), which seems like it would satisfy the intention of this PR 👍 so I think that will be useful to the use case you have here! In the mean time, as our tech is now, we don’t think it’s the right thing to build an entire app as a plugin, but I have some advice below on making it easy to implement in your fork if you like! These are our main reasons for holding off on this plugin build in the core repository:
If you want make a plugin build like this in your own fork, however, it can actually be just a small change, so hopefully it’s easy to maintain 🙂 Here are some tips:
Hopefully these things help you out and make it easy to do what you want to do! 🙏 Thanks again for your input here! We value hearing your use case, and it’s helpful to get new ideas and perspectives. Let us know if you have any other questions or ideas! |
Hi @KaiVandivier Thanks for your thoughtful response! It helps a lot. We have been testing some of your ideas and we have some comments:
|
Hey Adrian! Glad to hear the plugin is working for you! 🎉 Ah yeah, I forgot to mention the For the deep linking, that’s right that you would use plugin.html for the iframe src 👍 for attributeOptionCombo, is that for the Data Approval app? I’m not very familiar with the app myself, but it doesn’t seem to use an attribute option combo anywhere — let me know if there’s something else that you mean by that, and I can inquire further 🔍 For the Data Entry app, the attributeOptionCombo can be deep-linked using the query param format Happy to help out with anything else if I can 🙂 |
Hi @KaiVandivier ! Sorry for the delay, we finally got the opportunity to integrate Data Approval as an iframe in one of our apps ( using query strings, not props) and is working great. We just needed the minor modifications we discussed. Thanks for all your help!
As you know, aggregated data values are associated to some attributeOptionCombo (one of the COCs of the dataSet categoryCombo), so typically we will need to disaggregate on these dimension to approve the data. In 2.36, the old Data Approval App showed a selector: However, that capability does not exist in the new React Data Approval App (or we have missed it!) |
Ahh - am I understanding right that you're looking for that feature in the new Approvals app, where you can disaggregate data by the AoC too? (Sorry if I misunderstood earlier!) If so, I can do a little research to find out why that isn't included in the new app, and if it's possible to add! 🙂 |
Yes, that would be great! For the moment, we've added the It was our understanding that the backend already supports approving by the tuple orgUnit/dataSet/aoc/period: Cool, we didn't know about this one, we'll be watching it! (@adrianq) |
@amcgee @KaiVandivier @Birkbjo This is just a proof of concept of what we have been discussing these days. It is very rough - we are still keeping the header, button, etc. which ideally we would like to hide with some props (this can be done in our fork as I understand it may not be interesting for you). For us, it would be great if we could have this infrastructure in capture-app, dashboard and potentially some other apps. This is how it looks like in our app. I am happy to show you the example in the DHIS2 instance if we have time later. Thank you!
@tokland thanks for this!